-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enzyme: simplify via mixedduplicated #483
Conversation
@@ -150,7 +126,7 @@ module EnzymeExt | |||
args2 = ntuple(Val(N)) do i | |||
Base.@_inline_meta | |||
if args[i] isa Active | |||
Duplicated(Ref(args[i].val), arg_refs[i]) | |||
EnzymeCore.MixedDuplicated(args[i].val, arg_refs[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct for the CPU, but I think we won't be able to pass MixedDuplicated
to the GPU.
Or at least we need an Adapt rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not have those already like we do for the other annotations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently not, now we do here: EnzymeAD/Enzyme.jl#1551
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should double check that this adapts to what we want on CUDA. I don't know if CuRef is right here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure but this is code wouldn’t affect any currently landed cuda support (this is only reverse mode).
So we should likely explore that in michel’s PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what about non CUDA GPUs?
@vchuravy in interim could we get this merged/tagged (is blocking CliMA/Oceananigans.jl#3618 / https://buildkite.com/clima/oceananigans/builds/16126#01903c25-aa5f-4e96-9088-70984b90ebe4 seemingly) |
@wsmoses just so that you aware, this is what |
No description provided.